Treat assignments in unreachable code as local bindings by hsusul · Pull Request #11666 · microsoft/pyright

Henry Su (hsusul) · 2026-08-20T17:40:28Z

Summary

Fixes Local variables in unused code are not detected #11449.
CPython makes a name local for the whole function if it is assigned anywhere in the function body, including after return.
The binder skipped unreachable statements, so those names were treated as globals and UnboundLocalError was missed. Nested nonlocal lookups into such names also failed.

Test plan

npx jest typeEvaluator2.test.ts -t "Unbound" --forceExit (from packages/pyright-internal)
npx jest typeEvaluator1.test.ts -t "Unreachable1" --forceExit
Confirm a read before an assignment that appears after return reports an unbound variable in the language server

CPython treats any assignment in a function as making the name local, even after a return. Skipping those statements left later-assigned names looking global and hid UnboundLocalError. Fixes microsoft#11449

Rich Chiodo (rchiodo) · 2026-08-24T18:11:38Z

🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR.

Rich Chiodo (rchiodo) · 2026-08-24T18:19:00Z

    GlobalNode,
    IfNode,
    ImportAsNode,
+    ImportFromAsNode,


Issue · Please address or respond

ImportFromAsNode is unused by this change, so this will fail unused-import checks. Remove the import.

Rich Chiodo (rchiodo) · 2026-08-24T18:19:05Z

+        return false;
+    }
+
+    override visitFunction(node: FunctionNode): boolean {


Warning · Non-blocking recommendation

This walker skips function/class headers but descends into lambda bodies. That misses assignment expressions in decorators, defaults, and class bases that bind in the enclosing scope, while incorrectly treating assignment expressions in a lambda body as enclosing-scope bindings. Please handle those scope boundaries explicitly and add regressions for both cases.

Rich Chiodo (rchiodo) · 2026-08-24T18:19:09Z

+            this._bindName(node.d.target);
+        }
+        return true;
+    }


Issue · Please address or respond

UnreachableNameBinder does not process global or nonlocal declarations. Consequently, an unreachable global x; x = ... or nonlocal x; x = ... binds the assignment directly in the current scope, incorrectly making it local. Preserve the declarations' binding semantics before binding targets and cover unreachable declaration cases.

Rich Chiodo (rchiodo) · 2026-08-24T18:19:14Z

Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified.

Rich Chiodo (rchiodo) · 2026-08-24T19:13:47Z

+// local for the entire function). Nested functions and classes are skipped
+// because DummyScopeGenerator already created their scopes.
+class UnreachableNameBinder extends ParseTreeWalker {
+    constructor(


Warning · Non-blocking recommendation

Unreachable global and nonlocal directives are not processed, so a later assignment can be bound to the current local scope rather than the declared scope. Handle these directives before binding targets and add focused unreachable-directive coverage.

Rich Chiodo (rchiodo) · 2026-08-24T19:13:51Z

+
+    override visitPatternCapture(node: PatternCaptureNode): boolean {
+        this._bindName(node.d.target);
+        return false;


Warning · Non-blocking recommendation

UnreachableNameBinder has no lambda scope boundary. Walking lambda: (x := 1) will bind x in the enclosing scope even though the assignment expression belongs to the lambda's scope. Skip lambda bodies and add a regression case.

Rich Chiodo (rchiodo) · 2026-08-24T19:13:56Z

+    override visitTypeAlias(node: TypeAliasNode): boolean {
+        this._bindName(node.d.name);
+        return false;
+    }


Warning · Non-blocking recommendation

Returning false for nested functions and classes skips decorators, default values, bases, and keywords, which are evaluated in the enclosing scope. Assignment expressions in those header expressions therefore miss their enclosing binding; traverse the headers while excluding only nested bodies.

Rich Chiodo (rchiodo) · 2026-08-24T19:14:00Z

+        this._bindName(node.d.target);
+        return false;
+    }
+}


Warning · Non-blocking recommendation

This new visitor duplicates the primary binder's syntax and scope classification, and the missing directive and scope-boundary cases already demonstrate drift. Reuse or centralize the scope-aware binding classification so future syntax additions do not update only one path.

Rich Chiodo (rchiodo) · 2026-08-24T19:14:04Z

Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified.

Rich Chiodo (rchiodo)

Approved via Review Center.

Rich Chiodo (rchiodo)

Please address the comments in the binder.ts

Rich Chiodo (rchiodo) · 2026-08-25T18:58:26Z

Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified.

Henry Su (hsusul) · 2026-08-26T04:43:26Z

Addressed the binder feedback in 3268d67:

removed the unused ImportFromAsNode import;
process unreachable global and nonlocal directives before binding targets;
traverse enclosing-scope function, class, and lambda header expressions while excluding nested bodies;
added regressions for unreachable directives, lambda body/default boundaries, function defaults/decorators, and class bases/decorators.

Validation passed for the focused Unbound tests (7/7), neighboring Unreachable1 test, pnpm run check, pnpm run typecheck, and git diff --check.

The full internal Jest run passed 69/71 suites and 2,716/2,718 tests. The two failures were in unrelated areas: DataClassReplace1 in typeEvaluator4.test.ts and Builtins1 in typeEvaluator1.test.ts. Neither failure touches the changed files or binder behavior in this revision.

I kept the broader binding-classification centralization suggestion out of this targeted correctness fix to avoid expanding the PR beyond the demonstrated scope-boundary and directive issues.

Rich Chiodo (rchiodo) · 2026-08-26T12:11:59Z

+        this._bindName(node.d.target);
+        return false;
+    }
+}


Warning · Non-blocking recommendation

UnreachableNameBinder independently classifies binding syntax and scope boundaries. Centralize or reuse the primary binder's scope-aware classification so future syntax changes cannot silently diverge.

[verified]

Rich Chiodo (rchiodo) · 2026-08-26T12:12:04Z

Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified.

Bind assignments in unreachable code so they still create locals.

be66ed7

CPython treats any assignment in a function as making the name local, even after a return. Skipping those statements left later-assigned names looking global and hid UnboundLocalError. Fixes microsoft#11449

Rich Chiodo (rchiodo) reviewed Aug 24, 2026

View reviewed changes

Rich Chiodo (rchiodo) added the review-auto:changes-requested Automated review: posted blocking findings to address. label Aug 24, 2026

Rich Chiodo (rchiodo) reviewed Aug 24, 2026

View reviewed changes

Rich Chiodo (rchiodo) approved these changes Aug 24, 2026

View reviewed changes

Rich Chiodo (rchiodo) added review-auto:approved Automated review: no blocking findings (approval posted). and removed review-auto:changes-requested Automated review: posted blocking findings to address. labels Aug 24, 2026

Rich Chiodo (rchiodo) requested changes Aug 24, 2026

View reviewed changes

Rich Chiodo (rchiodo) added review-auto:changes-requested Automated review: posted blocking findings to address. and removed review-auto:approved Automated review: no blocking findings (approval posted). labels Aug 25, 2026

fix(binder): preserve unreachable binding scopes

3268d67

Rich Chiodo (rchiodo) reviewed Aug 26, 2026

View reviewed changes

Rich Chiodo (rchiodo) approved these changes Aug 26, 2026

View reviewed changes

Rich Chiodo (rchiodo) added review-auto:approved Automated review: no blocking findings (approval posted). and removed review-auto:changes-requested Automated review: posted blocking findings to address. labels Aug 26, 2026

Uh oh!

Conversation

Henry Su (hsusul) commented Aug 20, 2026

Summary

Test plan

Uh oh!

Rich Chiodo (rchiodo) commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) commented Aug 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) commented Aug 24, 2026

Uh oh!

Rich Chiodo (rchiodo) left a comment

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) left a comment

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) commented Aug 25, 2026

Uh oh!

Henry Su (hsusul) commented Aug 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) commented Aug 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Rich Chiodo (rchiodo) commented Aug 24, 2026 •

edited

Loading